Skip to content

Quote constant extraction #3697

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 22, 2018

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Dec 23, 2017

Add quoted.Contant.unapply.

val e = '(3) // '
e match {
  case Constant(n) => // get n with value 3
  case _ => // e is not a constant
}

Based on #3685

@nicolasstucki nicolasstucki force-pushed the quote-constant-extraction branch from f04dae7 to a80c141 Compare December 23, 2017 19:59
@nicolasstucki nicolasstucki force-pushed the quote-constant-extraction branch 3 times, most recently from 6821302 to 30188fc Compare January 15, 2018 07:17
@nicolasstucki nicolasstucki requested review from odersky and liufengyun and removed request for odersky January 15, 2018 16:25
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A conceptual question, how does Expr[T] going to interact with tpd.Tree?

I see we have ConstantExpr for literals, what about other trees, e.g. Apply, Block and If-Else? Are we going to create another hierarchy of Expr[T] trees for extractions?

@nicolasstucki nicolasstucki force-pushed the quote-constant-extraction branch from 30188fc to 10a207d Compare January 19, 2018 07:38

object Constant {
def unapply[T](expr: Expr[T])(implicit runner: Runner[T]): Option[T] = runner.toConstantOpt(expr)
}
Copy link
Contributor

@liufengyun liufengyun Jan 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should leave design space for other extractors as well. Some thoughts:

  • it's better all extractors are located in the same file (as there aren't many)
  • the name runner doesn't make sense to programmers.
    • FYI, in gestalt it's called Toolbox, in macros it's called Universe.
  • ConstantExpr can be removed, and Liftables could be implemented with an implicit Runner

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needing an implicit Runner is something that should be avoided. That would create an unnecessary dependency on the compiler/toolbox. In the current design, if you only quote, splice and inline the user never needs to add the runner.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. I just want to point out another logically consistent design. As long as the design tradeoffs are clear, I think it's fine to optimise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In essence, the runner is equivalent to the toolbox except that we only use it to interpret expression at runtime (compile and run, show and deconstruction). The constructor are the quotes themselves, '(if (~cond) ~e1 else ~e2) is the constructor for an if expression. The only missing constructor is for literal constants from runtime primitive values, which we special case in ConstantExpr. It is true that having ConstantExpr is more performant but it is not an optimization. Thought we could decide to compile '(5) to a ConstantExpr to avoid the pickling memory and runtime costs.

@nicolasstucki nicolasstucki force-pushed the quote-constant-extraction branch from 10a207d to a6da04f Compare January 21, 2018 11:11
@nicolasstucki
Copy link
Contributor Author

rebased

@nicolasstucki
Copy link
Contributor Author

CLA is failing for no reason but CI passes

@nicolasstucki nicolasstucki merged commit 6ad8d72 into scala:master Jan 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants